Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix #442: use reflectwalk to walk through circuit structures without building a Schema #444

Merged
merged 18 commits into from
Jan 23, 2023
Merged

fix #442: use reflectwalk to walk through circuit structures without building a Schema #444

merged 18 commits into from
Jan 23, 2023

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Jan 21, 2023

Closes #442 #433 #390.

Description

Introduces schema.Walk which behaves similarly to schema.Parse except it doesn't build a Schema, just visit the leaves.

Known issues:

  • we still need schema.Parse to handle json witnesses (play.gnark.io), and it won't handle all circuit structures well
  • schema.Walk doesn't like it if the leaf we are looking for is a reflect.Ptr. Only happens in... witness where we want to identify zero values in json will probably revisit when it's warranted.

Benchmark

Old (parse full schema) vs new (walk):

BenchmarkLargeSchema/walk-10                        4074            277284 ns/op           15456 B/op       4000 allocs/op
BenchmarkLargeSchema/parse-10                          1        2127391458 ns/op        3490130176 B/op 66707400 allocs/op
BenchmarkArrayOfSliceOfStructSchema/walk-10                    1        6265857250 ns/op        802062272 B/op  100257706 allocs/op
BenchmarkArrayOfSliceOfStructSchema/parse-10                   1        13797889083 ns/op       21742293688 B/op        351920063 allocs/op

@gbotrel gbotrel added this to the v0.8.0 milestone Jan 21, 2023
@gbotrel gbotrel requested a review from ivokub January 21, 2023 00:07
ivokub
ivokub approved these changes Jan 23, 2023
View reviewed changes
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be OK.

I understand the change introduces a new way for walking the circuit struct to call some handler on the leafs (for initialising the variables). We do not otherwise change the behaviour (e.g. for compile we still walk several times to first get counts of variables and then build variables separately)? I think it is OK as walking for counting is quite fast.

There is now ParseDeprecated and LeafHandlerDeprecated. I didn't understand fully why they are still necessary, are they for marshalling/unmarshalling? Do we plan to fully deprecate or rewrite?

I do like that the visibility handling became simpler. Seems that it covers all the cases. At some point I fixed some problem when name was set but visibility not and then the parser assumed Secret visibility instead of inhereting, but it seems this is also now covered.

@gbotrel
Copy link
Collaborator Author

gbotrel commented Jan 23, 2023

Yes behavior is unchanged, we may have some perf issues with very large complex circuits (with multi dimension slices and structs) at compile time, but nothing urgent to fix for now.

ParseDeprecated I will try to kill it and reuse a walker pattern to ensure we use the same exact code (reflection part). The only part where we use this method (probably better named as schema.New) is to marshal/unmarshall json witness. So for us, in play.gnark.io . --> not critical if slow for large circuits.

@gbotrel gbotrel merged commit 475b5dc into develop Jan 23, 2023
@gbotrel gbotrel deleted the gbotrel/issue442 branch January 23, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivokub ivokub ivokub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@gbotrel @ivokub